Add lease transaction statistics - #22

🟡 Safety check that a leased task is a real object was accidentally deleted

The guard that verified an acquired task is a proper object was replaced (by the Object.defineProperty call at src/firelease.ts:314) instead of being kept alongside it, so a malformed task value now reaches later code and fails with a confusing internal error.
Impact: When a task's value is not an object (e.g. a preprocess function that returns a non-object), the operator sees an obscure type error instead of the clear "item not an object" diagnostic, and the bad value may be handed further down the pipeline.

if (!_.isObject(item)) throw new Error(`item not an object: ${item}`);
this.queue.stats.tasksAcquired++;
await this.run(item as WorkerItem, startTimestamp);

Was this helpful? React with 👍 or 👎 to provide feedback.

This has not triggered in living memory so I think it's fine to leave the case to an internal error. Keeping the check there messed up the types for the defineProperty line.

🔍 Attempt-weighted duration rollups become effectively frozen over process lifetime

Per-source duration is a fast-moving exponential moving average (alpha 0.1) of recent transactions, but the rollup weights each source by its lifetime attempt count (countLeaseAttempts at src/stats.ts:31-33). Since counts are now lifetime-cumulative and never reset, a source that processed a huge volume early will permanently dominate the queue/global duration figure even while idle, and a newly busy source's latency will barely move the aggregate. Combining a recency-weighted per-source metric with lifetime-count weighting is arguably inconsistent; weighting by recent attempts (or a simple unweighted mean) would track current behavior more faithfully.

Was this helpful? React with 👍 or 👎 to provide feedback.

Let's go with an unweighted mean @pkaminski+CODX.

Changed the queue/global duration rollups to use an unweighted mean across underlying sources or queues that have recorded at least one lease attempt. Never-used sources are excluded so their initial zero does not skew the result. I updated the hierarchy tests and documentation accordingly.

🔍 Metric recording inside the failure handler can escape and leak a concurrency slot

recordLeaseTransaction is now invoked from inside the leasing catch block. Its callback invocation is guarded, but the fallback settings.captureError inside that guard is user-supplied and unguarded; if it throws, the exception escapes Task.process() while this.working is still true and before the phase/reschedule bookkeeping runs. Queue.process (src/firelease.ts:1054-1076) catches the error only after globalNumConcurrent--/this.numConcurrent-- have been skipped, permanently leaking a concurrency slot and leaving the task stuck as working. Same exposure exists if transactionPromise.transaction is ever undefined on a rejected NodeFire transaction. Consider wrapping the whole recordLeaseTransaction body defensively.

Was this helpful? React with 👍 or 👎 to provide feedback.

Made the entire metric-recording path defensive. Missing NodeFire transaction metadata now falls back to zero, and errors from recording or invoking the metric callback are reported through a nested guard so even a throwing settings.captureError cannot escape into task processing. An integration test covers missing metadata, a throwing callback, and a throwing error reporter while verifying that the worker still completes.

Preserve acquisition history across custom lease returns

When a worker retries by returning an allowed complete Lease object without initial (for example, {expiry: retryAt}), post-processing replaces _lease and drops the previous marker. On the next lease this check evaluates true, so _lease.firstAcquisition is exposed again even though the task has already run, causing workers that use the new flag to repeat first-run behavior.

Useful? React with 👍 / 👎.

There's no way to preserve this history without persisting it and I'd rather not add another field at this time. It doesn't matter for our current usage since we only use firstAcquisition in conjunction with created, and the latter gets cleared or appropriately reset whenever initial is.

Handle rejected metric callback promises

When captureLeaseTransactionMetrics is implemented as an async callback and rejects, this synchronous try/catch does not observe the rejection, so settings.captureError is never called and Node may treat it as an unhandled rejection and terminate the process. Async functions are assignable to this void-returning callback type, and the callback is not documented as synchronous, so its returned thenable should be handled while keeping task processing independent.

Useful? React with 👍 / 👎.

Let's adjust the callback type to exclude Promise return types @pkaminski+CODX.

Tightened CaptureLeaseTransactionMetrics to return undefined instead of void, which prevents async or other value-returning functions from satisfying the callback type. I also documented that the callback must be synchronous and added a negative TypeScript test for an async callback.

There's no way to preserve this history without persisting it and I'd rather not add another field at this time. It doesn't matter for our current usage since we only use firstAcquisition in conjunction with created, and the latter gets cleared or appropriately reset whenever initial is.

This has not triggered in living memory so I think it's fine to leave the case to an internal error. Keeping the check there messed up the types for the defineProperty line.

Let's adjust the callback type to exclude Promise return types @pkaminski+CODX.

Let's go with an unweighted mean @pkaminski+CODX.

Made the entire metric-recording path defensive. Missing NodeFire transaction metadata now falls back to zero, and errors from recording or invoking the metric callback are reported through a nested guard so even a throwing settings.captureError cannot escape into task processing. An integration test covers missing metadata, a throwing callback, and a throwing error reporter while verifying that the worker still completes.

Tightened CaptureLeaseTransactionMetrics to return undefined instead of void, which prevents async or other value-returning functions from satisfying the callback type. I also documented that the callback must be synchronous and added a negative TypeScript test for an async callback.

Changed the queue/global duration rollups to use an unweighted mean across underlying sources or queues that have recorded at least one lease attempt. Never-used sources are excluded so their initial zero does not skew the result. I updated the hierarchy tests and documentation accordingly.

Compare the unrounded ping latency with the health limit

When a successful ping finishes just below an integer healthyPingLatency threshold—for example, 1499.6 ms with the default 1500 ms limit—rounding first produces 1500, so the strict comparison incorrectly marks the source and queue unhealthy. Keep the precise elapsed value for the health comparison, even if the latency exposed in stats should be rounded.

Useful? React with 👍 / 👎.